Cap node processing at 10mil and log issue - #3628
Conversation
noahfalk
left a comment
noahfalk
left a comment
There was a problem hiding this comment.
Please add some description to the PR saying what issue(s) are being addressed. You have some understanding because you directly encountered the problem but nobody else looking at this change in the future will have that same context.
| eventPipeDataPresent = true; | ||
|
|
||
| if (gcNum < 0 && data.Depth == 2 && data.Type != GCType.BackgroundGC) | ||
| if (gcNum < 0 && data.Depth == 2 && data.Type != GCType.BackgroundGC && data.Reason == GCReason.Induced) |
There was a problem hiding this comment.
Can you either update the PR description or remove this part of the change? I can't determine how this part of the change relates.
There was a problem hiding this comment.
This was missed from the GC dump logic at: https://github.com/microsoft/perfview/blob/e2696ec466d4cacdfc057adf0e09ce22728573a5/src/EtwHeapDump/DotNetHeapDumpGraphReader.cs#L224
There was a problem hiding this comment.
Removed it mostly to revert to prior behavior - although this is technically a bug and can cause an early detach from the EP session.
There was a problem hiding this comment.
Removed it mostly to revert to prior behavior
I don't know what you mean? GitHub doesn't show the change has been removed in the latest version of your PR? I've still got the same request as before - lets either remove this edit from the PR or add text to summary commit message/PR description saying what issue is being fixed here :)
|
Could you apply the same updates to https://github.com/dotnet/diagnostics/blob/main/src/Microsoft.Diagnostics.Monitoring.EventPipe/GCDump/EventGCDumpPipeline.cs |
davmason
left a comment
davmason
left a comment
There was a problem hiding this comment.
+1 to capturing the context for the change in the PR, otherwise LGTM
4 participants
Return to pre-large heap support cap of 10 million nodes. Prior to #3475 we capped analysis at 10M nodes. After removing the cap, we often find issues in processing data. There's often an imbalance between node's edge count and the number of available edges. The problem is this error yields no gcdump, so we're adding the cap back until the edge imbalance can be figured out or the dumper can be made resilient. This still hides the buggy behavior around large graphs until it gets further investigation.